Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ACS-5293] changed non-unique data automation ids #3226

Merged
merged 3 commits into from
May 25, 2023
Merged

[ACS-5293] changed non-unique data automation ids #3226

merged 3 commits into from
May 25, 2023

Conversation

SheenaMalhotra182
Copy link
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation
  • Other... Please describe:

What is the current behaviour? (You can also link to an open issue here)
non-unique data-automation-id="menu-item-title" was being used in the toolbar-menu component to render create/upload buttons.

What is the new behaviour?
non-unique data-automation-id="menu-item-title" is replaced with unique ids.

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:
https://alfresco.atlassian.net/browse/ACS-5293

@SheenaMalhotra182 SheenaMalhotra182 marked this pull request as ready for review May 24, 2023 12:08
MichalKinas
MichalKinas reviewed May 24, 2023
View reviewed changes
Copy link
Contributor

@MichalKinas MichalKinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see a lot of W/element - more than one element found for locator By(css selector, .app-search-button) - the first result will be used messages. I think that taking care of this will also speed up test execution. Please investigate

@SheenaMalhotra182
Copy link
Contributor Author

I can see a lot of W/element - more than one element found for locator By(css selector, .app-search-button) - the first result will be used messages. I think that taking care of this will also speed up test execution. Please investigate

this class .app-search-button was being used in two components (SearchInputComponent and SearchInputControlComponent). Due to this, W/element - more than one element found for locator By(css selector, .app-search-button) message was coming in e2e runs. To avoid this, I have specified the selector by adding the container component. Post this change, the above message is not recurring in e2e runs.

@DenysVuika DenysVuika merged commit 39aa789 into develop May 25, 2023
@DenysVuika DenysVuika deleted the ACS-5293 branch May 25, 2023 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichalKinas MichalKinas MichalKinas approved these changes

@DenysVuika DenysVuika DenysVuika approved these changes

@rbahirsheth rbahirsheth Awaiting requested review from rbahirsheth

@eromano eromano Awaiting requested review from eromano

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SheenaMalhotra182 @DenysVuika @MichalKinas